use receptorNames (oid=otherNames) in tls bootstrap; fixed associated unit tests for receptor and receptorctl #578
Conversation
|
I just tried this and I'm not sure if I'm doing something wrong, but I'm still able to join the mesh after modifying my receptor config to include a mismatching node name. If you are around today and have time, let's jump on a call and look at this together. |
…ociated unit tests for receptor and receptorctl
sarabrajsingh
force-pushed
the
feature/issue-420-receptor-names-node-ids
branch
from
2238548 to
4a0c940
Compare
pkg/netceptor/tlsconfig.go
Outdated
| RequireClientCert bool `required:"false" description:"Require client certificates" default:"false"` | ||
| ClientCAs string `required:"false" description:"Filename of CA bundle to verify client certs with"` | ||
| PinnedClientCert []string `required:"false" description:"Pinned fingerprint of required client certificate"` | ||
| SkipReceptorNamesCheck bool `required:"false" description:"if enabled, validate cert using ReceptorNames OID in Certificate"` |
There was a problem hiding this comment.
the description for this is tricky, I think we want to emphasize this boolean only controls the check at startup, not during the quic TLS handshake (where the receptor names will always be checked, regardless of how this boolean is set)
maybe something like,
Skip verifying Receptor node ID in certificate at startup
There was a problem hiding this comment.
done @fosterseth
sarabrajsingh
changed the title
|
Let's get the "fix lint" and "fix tests" commits rebased out of the history here. Those should get squashed into the commits that introduced whatever problems you were addressing. |
sarabrajsingh
force-pushed
the
feature/issue-420-receptor-names-node-ids
branch
2 times, most recently
from
fe7edae to
09e59ce
Compare
…erverCfg and clientCfg
sarabrajsingh
force-pushed
the
feature/issue-420-receptor-names-node-ids
branch
from
09e59ce to
f6f22df
Compare
|
@shanemcd done |
|
Awesome, thank you. @fosterseth have you pulled this down and played around? |
| return err | ||
| } | ||
|
|
||
| if !found { |
There was a problem hiding this comment.
we still need to return without error here if len(receptorNames) == 0
if I create a cert without a nodeID, and use that on a config with node ID foo, but I see this error
[sbf@fedora sockceptor]$ ./receptor -c foo.yml
Error: MainInstance.nodeID=foo not found in certificate name(s); names found=[]; cfg section=server2; server cert=/home/sbf/sockceptor/certs/bob.crt
we want this to be permissible
There was a problem hiding this comment.
@fosterseth - just thinking about this before i write a patch. would it be more optimal for the consumer to just flip the bit for the SkipReceptorNamesCheck flag to true if they give a ReceptorName/OID="" and skip the check altogether?
| RootCAs string `required:"false" description:"Root CA bundle to use instead of system trust"` | ||
| InsecureSkipVerify bool `required:"false" description:"Accept any server cert" default:"false"` | ||
| PinnedServerCert []string `required:"false" description:"Pinned fingerprint of required server certificate"` | ||
| SkipReceptorNamesCheck bool `required:"false" description:"if enabled, skip verifying ReceptorNames OIDs in certificate at startup"` |
There was a problem hiding this comment.
golang will default this to false, but for consistency and clarify I think we can add default:"false" , similar to InsecureSkipVerify
To make the description a little shorter, we could omit "if enabled," and just start with "Skip.."
|
Note: if users that have set up tls-server and tls-client to be used for TCP connections only, and thus may not have the receptor name in the cert, they should add |
…d SkipReceptorNamesCheck in tlsServerConfig and tlsClientConfig structs
sarabrajsingh
force-pushed
the
feature/issue-420-receptor-names-node-ids
branch
from
7925d45 to
0466af0
Compare
upstream issue #420
added otherName (SAN) checks to
tlsconfig.gousingutils.ReceptorNames()@fosterseth @thenets @shanemcd please re-review!